[Tests] Increase QMK test coverage #13789
Conversation
KarlK90
force-pushed
the
add-qmk-unit-tests
branch
from
b30de2f to
c6b2db0
Compare
KarlK90
force-pushed
the
add-qmk-unit-tests
branch
2 times, most recently
from
ab5b42e to
b29bde3
Compare
KarlK90
changed the title
KarlK90
changed the title
|
This is now ready for review and a good starting point to add more test-cases.
|
|
Every QMK configuration that needs a different binary is now split out into an individual testing binary, so all tests can be run simultaneously. 💪 |
KarlK90
force-pushed
the
add-qmk-unit-tests
branch
2 times, most recently
from
319044d to
9f9a9a6
Compare
|
Can the "in progress" label be removed here? |
|
Yes it can! |
|
Looks like this is erroring out on: Running this from the docker container. |
|
Yes, this is expected, as the test documents false buggy behavior and was added in d50dcaf. |
|
Well, specifically it is causing one of the tests to fail, which is my concern. |
|
Yup, that is the one I added in the commit. It is an edge-case which doesnt happen to often on ansi-us layouts and the like. My reasoning was that I rather document the expected behaviour then what QMK currently does. Which is removing the modifier from the report on a layer change, altough it should not be modified because the key adding the modifier is still pressed. This results on a german layout in ((((<layer release>8888 instead of ((((<layer release>(((( |
|
|
||
| void TestLogger::reset() { this->m_log.str(""); }; | ||
|
|
||
| void TestLogger::print_log() { std::cerr << this->m_log.str(); } |
There was a problem hiding this comment.
It would be nice if this somehow took advantage of #13556, rather than it being its own entity.
Maybe binding int8_t sendchar(uint8_t c) to print to the same buffer, or using the debug macros instead of piping to std::cerr?
From a quick skim of the PR, the high level is that you want to only dump log on errors? I could potentially see it being none/errors/all, with a nicer interface than the current DEBUG=.
KarlK90
force-pushed
the
add-qmk-unit-tests
branch
from
674763f to
f15f680
Compare
tests/basic/test_action_layer.cpp
Outdated
| set_keymap({layer_key}); | ||
|
|
||
| /* Press and release MO, nothing should happen. */ | ||
| /* TODO: Should not send empty report! */ |
There was a problem hiding this comment.
If you can come up with better wording, by all means -- it's just so it's obvious that it's not a problem with the test, but with QMK itself.
Also, please change the other comment locations that have the same issue.
| /* TODO: Should not send empty report! */ | |
| /* TODO: QMK currently sends an empty report even if nothing needs to be reported to the host! */ |
tests/basic/test_action_layer.cpp
Outdated
|
|
||
| set_keymap({layer_key}); | ||
|
|
||
| /* Press and release TG. */ |
There was a problem hiding this comment.
Please apply a similar transformation where appropriate.
| /* Press and release TG. */ | |
| /* Press TG. Layer state should not change as it's applied on release. */ |
tests/basic/test_action_layer.cpp
Outdated
| EXPECT_TRUE(layer_state_is(0)); | ||
| testing::Mock::VerifyAndClearExpectations(&driver); | ||
|
|
||
| /* TODO: Should not send empty report! */ |
There was a problem hiding this comment.
Please apply a similar transformation where appropriate.
| /* TODO: Should not send empty report! */ | |
| /* Release TG */ | |
| /* TODO: <as per whatever wording you come up with from previous comment> */ |
tests/basic/test_action_layer.cpp
Outdated
| EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); | ||
| layer_key.press(); | ||
| run_one_scan_loop(); | ||
| EXPECT_TRUE(layer_state_is(0)); /* This is wrong? */ |
There was a problem hiding this comment.
Arguably layer_state_is should probably be something like layer_state_has -- it checks for a specific layer, ignoring the state of others.
In this specific case, layer_state_is(1) should also be true, right?
There was a problem hiding this comment.
Exactly but it is actually 0 which should be false, very curios. I'll add the correct expected behavior but set the test to be ignored and collect those in a separate issue.
tests/basic/test_action_layer.cpp
Outdated
| // TEST_F(ActionLayer, LayerTapToggleWithKeypress) { | ||
| // TestDriver driver; | ||
| // KeymapKey layer_key = KeymapKey{0, 0, 0, TT(1)}; | ||
|
|
||
| // /* These keys must have the same position in the matrix, only the layer is different. */ | ||
| // KeymapKey regular_key = KeymapKey{0, 1, 0, KC_A}; | ||
| // set_keymap({layer_key, regular_key, KeymapKey{1, 1, 0, KC_B}}); | ||
|
|
||
| // /* Press TT. */ | ||
| // /* TODO: Should not send empty report! */ | ||
| // EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); | ||
| // layer_key.press(); | ||
| // run_one_scan_loop(); | ||
| // EXPECT_TRUE(layer_state_is(1)); | ||
| // testing::Mock::VerifyAndClearExpectations(&driver); | ||
|
|
||
| // EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_B))).Times(1); | ||
| // regular_key.press(); | ||
| // run_one_scan_loop(); | ||
| // EXPECT_TRUE(layer_state_is(1)); | ||
| // testing::Mock::VerifyAndClearExpectations(&driver); | ||
|
|
||
| // EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(1); | ||
| // regular_key.release(); | ||
| // run_one_scan_loop(); | ||
| // EXPECT_TRUE(layer_state_is(1)); | ||
| // testing::Mock::VerifyAndClearExpectations(&driver); | ||
|
|
||
| // /* TODO: Should not send empty report! */ | ||
| // EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(1); | ||
| // layer_key.release(); | ||
| // run_one_scan_loop(); | ||
| // EXPECT_TRUE(layer_state_is(0)); | ||
| // testing::Mock::VerifyAndClearExpectations(&driver); | ||
| // } |
There was a problem hiding this comment.
What reason prevents this scenario from working?
The sequence looks correct to me?
| } | ||
|
|
||
| TEST_F(DefaultTapHold, tap_mod_tap_hold_key_two_times) { | ||
| GTEST_SKIP() << "Keymap out of bounds access, uncomment for diagnostics!"; |
There was a problem hiding this comment.
Can you clarify, here? Testing bug? QMK bug?
| } | ||
|
|
||
| TEST_F(DefaultTapHold, tap_mod_tap_hold_key_twice_and_hold_on_second_time) { | ||
| GTEST_SKIP() << "Keymap out of bounds access, uncomment for diagnostics!"; |
There was a problem hiding this comment.
Can you clarify, here? Testing bug? QMK bug?
KarlK90
force-pushed
the
add-qmk-unit-tests
branch
from
f15f680 to
f057a5d
Compare
|
Some merge conflicts |
|
As discussed on Discord, reverted thru commit 7746aef. |
* qmk/develop: (80 commits) Remove use of __flash due to LTO issues (qmk#15268) Revert "handwired/split89 Layout Macro Refactor (qmk#15210)" (qmk#15284) New Keyboard: TGR Jane CE (qmk#14713) Portal 66 Layout Macro Refactor (qmk#15255) Pluckey: Fix QMK Configurator Implementation (qmk#15254) [Tests] Increase QMK test coverage take 2 (qmk#15269) Ignore exit codes for formatters (qmk#15276) [Keyboard] Disable features on SplitKB boards to fit under size (qmk#15262) Ignore exit codes for formatters (qmk#15275) Ignore deleted files when formatting codebase (qmk#15274) qmk format-python - filter for Python files (qmk#15271) Revert "[Tests] Increase QMK test coverage (qmk#13789)" [Tests] Increase QMK test coverage (qmk#13789) [Docs] Squeezing space out of AVR (qmk#15243) Add uint to char functions (qmk#15244) [Keyboard] Disable console on Keebio foldkb and iris rev3 (qmk#15260) layer_combo → sd_combo (qmk#15266) [Keymap] Disable console on Sofle default keymap (qmk#15261) [Keyboard] Enable LTO on viktus/sp_mini via keymap (qmk#15263) Macros in JSON keymaps (qmk#14374) ...
Description
This PR enhances the current
unit-testingintegration testing facilities and adds additional tests:keymap.cthe usage goes like this:Types of Changes
Issues Fixed or Closed by This PR
Checklist